Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix for pull request #2 #3

Merged
merged 3 commits into from Apr 17, 2012
Merged

Bug fix for pull request #2 #3

merged 3 commits into from Apr 17, 2012

Conversation

yamaneko1212
Copy link
Contributor

Bug fix for #2 Add genericdownloader as substitute of curl module.
Though I want to use fetch command via popen, it can't use HEAD method, so I write genericdownloader module. This module consists of urllib and httplib and behaves like curl module.
I check these changes in FreeBSD and MacOSX and they work well ;)

def update_line(self, current):
num_bar = int(current / 100.0 * (self._term_width - 5))
return '#' * num_bar + ' ' * (self._term_width - 5 - num_bar) + \
u' %3d' % int(current) + u'%\r'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't break the like here, screens are wide enough nowadays :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not so bad, it's because PEP8 said that limit all lines to a maximum of 79 characters.
http://www.python.org/dev/peps/pep-0008/#maximum-line-length
But, if you don't like above, i'll modify it like this.

    def update_line(self, current):
        num_bar = int(current / 100.0 * (self._term_width - 5))
        bars = u'#' * num_bar
        spaces = u' ' * (self._term_width - 5 - num_bar)
        percentage = u'%3d' % int(current) + u'%\r'
        return bars + spaces + u' ' + percentage

@saghul
Copy link
Owner

saghul commented Apr 17, 2012

Hi,

Thanks! The diff looks fine, I just added some minor comments. Actually I'm thinking about removing the Curl downloader, doing things via Popen seems bad if we can do it with the standard library. I'll do that once I merge your pull request.

@yamaneko1212
Copy link
Contributor Author

OK, I fixed them. Please check them.

Actually I'm thinking about removing the Curl downloader, doing things via Popen seems bad if we can do it with the standard library.

Nice!

@saghul saghul merged commit 9a4c85b into saghul:master Apr 17, 2012
@saghul
Copy link
Owner

saghul commented Apr 17, 2012

Done! I just merged your changes and then removed Curl :-) Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants